Guide users to use bulk status when an operation is still running in the background#6678
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
from
c9dcbd0 to
282d7ce
Compare
jordanverasamy
changed the title
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
10 times, most recently
from
ab6f464 to
2db0cfe
Compare
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3451 tests passing in 1397 suites. Report generated by 🧪jest coverage report action from df935ac |
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
5 times, most recently
from
cfa7d4b to
88b4de5
Compare
| watch: true, | ||
| }) | ||
|
|
||
| expect(watchBulkOperation).toHaveBeenCalledWith(mockAdminSession, createdBulkOperation.id) |
There was a problem hiding this comment.
(I don't think this assertion was providing actual value, since the thing we actually care about is that the success banner was rendered; I opted to just remove it instead of fixing the parameters on the assertion.)
| watch: true, | ||
| }) | ||
|
|
||
| expect(watchBulkOperation).toHaveBeenCalledWith(mockAdminSession, createdBulkOperation.id) |
There was a problem hiding this comment.
jordanverasamy
changed the title
bulk status when an operation is still running in the background
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
from
88b4de5 to
dd01d40
Compare
| ) | ||
|
|
||
| if (abortController.signal.aborted) { | ||
| renderInfo({ |
There was a problem hiding this comment.
Made this an info banner per the brief, but I do think it looks a bit odd being all grey, maybe a success banner would more appropriately convey to the user "nothing went wrong, all is still good and running nicely". I'm not really sure. Let me know what you think is correct!
There was a problem hiding this comment.
I agree a success banner would be better. Actually, why not showing exactly the same message as when you run it without --watch?
╭─ success ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Bulk operation started. │
│ │
│ Monitor its progress with: │
│ shopify app bulk status --id="gid://shopify/BulkOperation/5749231780082" │
│ │
│ • ID: gid://shopify/BulkOperation/5749231780082 │
│ • Status: CREATED │
│ • Created at: 2025-12-04T10:37:57Z │
│ │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
There was a problem hiding this comment.
Yeah, I considered this but decided it was a slightly different context so the most helpful possible messaging to the user is slightly different. I'll log this for Nick to think about though! TY!
There was a problem hiding this comment.
Since this is the Ctrl+C banner, I think info is correct, since it’s just informing you that the query is still running.
| handleCtrlC(input, key) | ||
| } | ||
| }, | ||
| {isActive: Boolean(isRawModeSupported)}, |
There was a problem hiding this comment.
This part is basically saying: only activate this behaviour if we're in an interactive terminal.
If STDIN isn't a TTY, for example if input is being piped, we're running in CI, or it's otherwise non-interactive, then isRawModeSupported will be false and the useInput hook will helpfully not do anything.
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
from
dd01d40 to
0d6aee9
Compare
jordanverasamy
changed the base branch from
graphite-base/6678
to
12-02-move_command_from_shopify_app_execute_to_shopify_app_bulk_execute_
jordanverasamy
changed the base branch from
12-02-move_command_from_shopify_app_execute_to_shopify_app_bulk_execute_
to
graphite-base/6678
jordanverasamy
force-pushed
the
graphite-base/6678
branch
from
942df86 to
502e0ae
Compare
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
from
0d6aee9 to
0a8232e
Compare
|
/snapit |
|
🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20251204100354Caution After installing, validate the version by running just |
gonzaloriestra
left a comment
gonzaloriestra
left a comment
There was a problem hiding this comment.
Nice improvement! Works great, just some suggestions and this small issue:
When you abort, it seems that it waits for the current polling sleep (up to 5 seconds) before actually finishing the command:
abort.mp4
Also, maybe you are already aware, but it's showing a duplicated ellipsis in the Starting task:
| const command = outputToken.cyan(`shopify app bulk status --id="${operationId}"`) | ||
| return outputContent`Monitor its progress with:\n${command}`.value |
There was a problem hiding this comment.
Instead of manually choosing a different color, there's a standard way to print commands, see this example.
There was a problem hiding this comment.
Wow, that's so nice, didn't know that! Adjusted, TY :)
| ) | ||
|
|
||
| if (abortController.signal.aborted) { | ||
| renderInfo({ |
There was a problem hiding this comment.
I agree a success banner would be better. Actually, why not showing exactly the same message as when you run it without --watch?
╭─ success ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Bulk operation started. │
│ │
│ Monitor its progress with: │
│ shopify app bulk status --id="gid://shopify/BulkOperation/5749231780082" │
│ │
│ • ID: gid://shopify/BulkOperation/5749231780082 │
│ • Status: CREATED │
│ • Created at: 2025-12-04T10:37:57Z │
│ │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
That's very interesting, it shouldn't... I specifically used
Yeah, that extra ellipsis will get cleaned up, you pointed it out before so we've got it tracked by https://github.com/shop/issues-api-foundations/issues/1149. Thanks!! |
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
from
0a8232e to
143c488
Compare
…k operation execution
jordanverasamy
force-pushed
the
jtv/ctrl-c-shows-how-to-resume
branch
from
143c488 to
df935ac
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/ui.d.ts@@ -335,6 +335,7 @@ export declare function renderTasks<TContext>(tasks: Task<TContext>[], { renderO
export interface RenderSingleTaskOptions<T> {
title: TokenizedString;
task: (updateStatus: (status: TokenizedString) => void) => Promise<T>;
+ onAbort?: () => void;
renderOptions?: RenderOptions;
}
/**
@@ -348,7 +349,7 @@ export interface RenderSingleTaskOptions<T> {
* ▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀
* Loading app ...
*/
-export declare function renderSingleTask<T>({ title, task, renderOptions }: RenderSingleTaskOptions<T>): Promise<T>;
+export declare function renderSingleTask<T>({ title, task, onAbort, renderOptions, }: RenderSingleTaskOptions<T>): Promise<T>;
export interface RenderTextPromptOptions extends Omit<TextPromptProps, 'onSubmit'> {
renderOptions?: RenderOptions;
}
packages/cli-kit/dist/private/node/ui/components/SingleTask.d.ts@@ -3,7 +3,8 @@ interface SingleTaskProps<T> {
title: TokenizedString;
task: (updateStatus: (status: TokenizedString) => void) => Promise<T>;
onComplete?: (result: T) => void;
+ onAbort?: () => void;
noColor?: boolean;
}
-declare const SingleTask: <T>({ task, title, onComplete, noColor }: SingleTaskProps<T>) => JSX.Element | null;
+declare const SingleTask: <T>({ task, title, onComplete, onAbort, noColor }: SingleTaskProps<T>) => JSX.Element | null;
export { SingleTask };
\ No newline at end of file
|
3 participants
WHY are these changes introduced?
https://github.com/shop/issues-api-foundations/issues/1095
We want to make sure it's clear to users when things are still running in the background.
WHAT is this pull request doing?
Let's helpfully print a nice message telling users when they can use the
bulk statuscommand, even if they CTRL+C out of an operation midway!We did this by:
onAbortcallback torenderSingleTask, which you can use to configure custom behaviour when the user quits mid-way through a single taskexecuteBulkOperationandwatchBulkOperationto use that newonAbortcallback to display a nice message when the user quits while watching a bulk operationHow to test your changes?
Without
--watch, you get a nice help message:With
--watch, if you abort using CTRL+C before it's done, you get a nice help message:CTRL+C-info-banner.mov
With
--watch, without aborting, same behaviour as before this PR: